-
Notifications
You must be signed in to change notification settings - Fork 346
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Always use the same permissions for youki dir #705
Always use the same permissions for youki dir #705
Conversation
Signed-off-by: Szymon Gibała <szymongib@gmail.com>
Signed-off-by: Szymon Gibała <szymongib@gmail.com>
Codecov Report
@@ Coverage Diff @@
## main #705 +/- ##
==========================================
- Coverage 70.77% 70.58% -0.19%
==========================================
Files 84 100 +16
Lines 11144 11349 +205
==========================================
+ Hits 7887 8011 +124
- Misses 3257 3338 +81 |
@Szymongib Thanks for your PR. First of all, this PR is enough valuable to merge. I think it would be better to add a unit test. WDYT? |
Signed-off-by: Szymon Gibała <szymongib@gmail.com>
46753a8
to
e085f42
Compare
Thanks, @utam0k. It is an interesting approach to mocking values for tests. I have added tests for the whole |
It seems the ci got failed 😭 Can you check it? |
.github/workflows/main.yml
Outdated
@@ -64,7 +64,7 @@ jobs: | |||
- run: sudo apt-get -y update | |||
- run: sudo apt-get install -y pkg-config libsystemd-dev libdbus-glib-1-dev libelf-dev libseccomp-dev | |||
- name: Run tests | |||
run: cargo test --all --all-features --no-fail-fast | |||
run: sudo cargo test --all --all-features --no-fail-fast |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to use sudo
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry that was my attempt to fix it. The issue is that CI tests do not run as a root, therefore rootless_required
always returns true
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This, unfortunately, does not work, so I will try to find a way through it, otherwise, I will delete the test_determine_root_path_non_rootless
test.
f57b1ef
to
52e8c0b
Compare
Signed-off-by: Szymon Gibała <szymongib@gmail.com>
52e8c0b
to
9af52a3
Compare
Signed-off-by: Szymon Gibała <szymongib@gmail.com>
Hey @utam0k, sorry for the inconvenience 😕 . I did not find a way to run tests as a root user so I added a check to skip the test if the user is not root https://github.com/containers/youki/pull/705/files#diff-6628fb893f86fbbacb0317d2125dc7777897c6eefac88ba293fb7e49bee75726R216-R219. Not sure if this is the best approach, let me know if you would prefer me to delete this test altogether. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 💯
Summary
This PR aligns permissions of
/run/youki
directory to700
.Relevant line in runc https://github.com/opencontainers/runc/blob/main/libcontainer/factory_linux.go#L49
Fixes #481